Skip to content

Fix source statistics - #4056

Open
dkachuma wants to merge 20 commits into
developfrom
dkachuma/fix-source-statistics
Open

Fix source statistics#4056
dkachuma wants to merge 20 commits into
developfrom
dkachuma/fix-source-statistics

Conversation

@dkachuma

@dkachuma dkachuma commented May 13, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Fixes: #4010

This PR addresses issues regarding the collection and merge of source flux statistics.

Changes Made

  • Time Advancement: Ensures that the time is properly advanced when a new collection is completed.
  • Statistics Reset: Forces a reset of statistics prior to merging to prevent stale or cumulative data from corrupting the merge output.
  • CSV File Handling: Updates the file-writing behaviour so the CSV file is appended to rather than overwritten each time, preserving historical collection data.
  • Fix the element count to exclude ghost cells.

@dkachuma dkachuma self-assigned this May 13, 2026
@dkachuma dkachuma added the type: bug Something isn't working label May 13, 2026
@dkachuma dkachuma linked an issue May 13, 2026 that may be closed by this pull request
Source flux statistics reporting errors in MPI #4010
Open
@dkachuma dkachuma added ci: run CUDA builds Allows to triggers (costly) CUDA jobs flag: ready for review ci: run integrated tests Allows to run the integrated tests in GEOS CI flag: no rebaseline Does not require rebaseline ci: run code coverage enables running of the code coverage CI jobs labels May 13, 2026
@dkachuma
dkachuma marked this pull request as ready for review May 13, 2026 18:43
@dkachuma
dkachuma requested review from MelReyCG May 13, 2026 19:50
MelReyCG
MelReyCG reviewed May 20, 2026
View reviewed changes
Comment thread src/coreComponents/physicsSolvers/fluidFlow/SourceFluxStatistics.cpp
Comment thread src/coreComponents/physicsSolvers/fluidFlow/SourceFluxStatistics.cpp
MelReyCG
MelReyCG reviewed May 21, 2026
View reviewed changes

@MelReyCG MelReyCG left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just a few comments requests

@MelReyCG

MelReyCG commented May 21, 2026

Copy link
Copy Markdown
Contributor

I was thinking, if a process is not involved in this computation, could it be excluded in some way? (if elementcount is 0 as an example)

@dkachuma

dkachuma commented May 22, 2026

Copy link
Copy Markdown
Contributor Author

@copilot Review and suggest code comments

@dkachuma
Add comments explaining max() usage for period start time in SourceFl…
a7f0894 
…uxStatistics

Agent-Logs-Url: https://github.com/GEOS-DEV/GEOS/sessions/bcea92d2-4f51-434e-9036-5f81b1953661

Co-authored-by: dkachuma <81433670+dkachuma@users.noreply.github.com>

Copilot AI commented May 22, 2026

Copy link
Copy Markdown
Contributor

@copilot Review and suggest code comments

Added explanatory comments at both max() call sites (commit a7f0894):

  • finalizePeriod() (line ~382): explains that MPI ranks without cells in the flux region can bypass gatherTimeStepStats() and lag behind, so MpiWrapper::max is used to ensure all ranks converge on the most advanced period start time.
  • combine() (line ~404): explains that the same lag can occur across individual flux stats, so LvArray::math::max picks the most advanced time when merging two WrappedStats.

@dkachuma

dkachuma commented May 22, 2026

Copy link
Copy Markdown
Contributor Author

Added explanatory comments at both max() call sites (commit a7f0894):

@copilot This is OK but I expected to have to approve the commit.

Copilot AI commented May 22, 2026

Copy link
Copy Markdown
Contributor

Added explanatory comments at both max() call sites (commit a7f0894):

@copilot This is OK but I expected to h...

As a coding agent, I commit and push changes directly when making edits — there's no approval step in between. If you'd prefer to review proposed changes before they're committed, you can describe what you'd like in a comment and I'll outline the changes first before applying them.

@jhuang2601
jhuang2601 self-requested a review as a code owner June 11, 2026 13:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@MelReyCG MelReyCG MelReyCG approved these changes
@rrsettgast rrsettgast Awaiting requested review from rrsettgast rrsettgast is a code owner
@tjb-ltk tjb-ltk Awaiting requested review from tjb-ltk tjb-ltk is a code owner
@OmarDuran OmarDuran Awaiting requested review from OmarDuran OmarDuran is a code owner
@jhuang2601 jhuang2601 Awaiting requested review from jhuang2601 jhuang2601 is a code owner

At least 1 approving review is required to merge this pull request.

Assignees

@dkachuma dkachuma

Labels

ci: run code coverage enables running of the code coverage CI jobs ci: run CUDA builds Allows to triggers (costly) CUDA jobs ci: run integrated tests Allows to run the integrated tests in GEOS CI flag: no rebaseline Does not require rebaseline flag: ready for review type: bug Something isn't working

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Source flux statistics reporting errors in MPI

4 participants

@dkachuma @MelReyCG @jhuang2601